Issue: _DEFAULT_CACHE_DIR = Path.home() / ".cache" / "strands" / "skills" is evaluated at module import time. In containerized environments (Lambda, Docker) where HOME may not be set, Path.home() raises RuntimeError.

Suggestion: Evaluate this lazily, e.g., inside clone_skill_repo when cache_dir is None. You could use a sentinel or simply inline Path.home() / ".cache" / "strands" / "skills" inside the function:

cache_dir = cache_dir or Path.home() / ".cache" / "strands" / "skills"

This also respects XDG_CACHE_HOME if you want to be a good citizen on Linux:

import os
cache_dir = cache_dir or Path(os.environ.get("XDG_CACHE_HOME", Path.home() / ".cache")) / "strands" / "skills"

Issue: http:// is included in URL prefixes, allowing skill repos to be cloned over plaintext HTTP. This exposes users to man-in-the-middle attacks where a malicious actor could inject arbitrary code into the cloned skill.

Suggestion: Remove "http://" from _URL_PREFIXES, or at minimum log a warning when an http:// URL is detected in clone_skill_repo. Git itself warns about this, but the SDK should guide users toward secure defaults (tenet: "the obvious path is the happy path").

Issue: Race condition — if two threads/processes call clone_skill_repo concurrently with the same URL, both pass the not target.exists() check and attempt to clone into the same directory. One will fail or produce a corrupt state.

Suggestion: Use an atomic pattern: clone to a temporary directory first, then os.rename() (atomic on the same filesystem) to the target path. If the target already exists by the time rename happens, just delete the temp clone. Something like:

import tempfile

with tempfile.TemporaryDirectory(dir=cache_dir) as tmp:
    tmp_target = Path(tmp) / "repo"
    # ... clone into tmp_target ...
    try:
        tmp_target.rename(target)
    except OSError:
        # Another process won the race — that's fine, use their clone
        pass

Issue: There is no mechanism to refresh a cached clone. When no ref is specified, the default branch is cloned. If the remote repo is updated, the user gets stale content indefinitely with no obvious way to refresh (other than manually deleting ~/.cache/strands/skills/).

Suggestion: Consider adding a force_refresh: bool = False parameter (or documenting the cache invalidation story clearly). At minimum, document in the docstring that users should delete the cache directory or pin a ref for reproducibility. This is especially important since the cache key is a hash — users can't easily find and delete the right directory.

Issue: The Raises section documents RuntimeError but the method also raises ValueError (via is_url check on line 377). Additionally, calling Skill.from_url() with a non-URL raises ValueError, but using the same string through AgentSkills(skills=["./path"]) treats it as a local path. This inconsistency could confuse users.

Suggestion: Add ValueError to the Raises section in the docstring.

Issue: The from_url ValueError uses an f-string in the exception message: f"url=<{url}> | not a valid remote URL". This is consistent with the structured logging style used elsewhere, but the AGENTS.md specifically says "Don't use f-strings in logging calls." Since this is an exception message (not a logging call), this is acceptable — but the logging calls throughout the module correctly use %s format, which is good.

However, the RuntimeError raised in clone_skill_repo at line 189 also uses an f-string: f"url=<{url}>, ref=<{ref}> | failed to clone...". This is fine since it's an exception message, not a log statement.

Issue: This docstring says "remote Git URL" but the implementation no longer uses git — it only supports https:// URLs fetched via urllib. This is stale from the v1 implementation.

Suggestion: Update to match the current implementation:

a path to a parent directory containing multiple skills, or an
HTTPS URL pointing to a SKILL.md file.

Issue: The AGENTS.md entry for _url_loader.py says "Remote URL loading, git clone, caching" which describes the v1 implementation. The module now does HTTPS fetching with no git or caching.

Suggestion: Update to:

│   │       └── _url_loader.py            # HTTPS skill fetching, GitHub URL resolution

Issue: resolve_to_raw_url only handles GitHub-specific URL patterns (github.com → raw.githubusercontent.com). For non-GitHub URLs that aren't direct links to a SKILL.md file, the URL is returned as-is. If a user passes https://gitlab.com/org/repo, the function will return it unchanged and fetch_skill_content will try to fetch an HTML page (GitLab repo page), not the raw SKILL.md content.

This isn't necessarily a bug — the function documents that non-GitHub URLs pass through — but the error experience could be confusing: the user gets a RuntimeError from a failed parse of HTML content, not a clear "unsupported host" message.

Suggestion: Consider logging a debug message for non-GitHub URLs to make it clear the SDK expects a direct link to SKILL.md content, or add a brief note in the Skill.from_url docstring that only GitHub URLs get automatic resolution and other hosts require a direct link to the raw SKILL.md file.

Issue: The S310 noqa suppression is appropriate here since the URL is validated to be https:// by the caller (Skill.from_url calls is_url first). However, fetch_skill_content itself doesn't validate that url starts with https:// — it could be called directly with any URL. Since this is a private module, the risk is low, but a defensive check would make the function safer independently.

Suggestion: Consider adding a quick guard:

if not url.startswith("https://"):
    raise ValueError(f"url=<{url}> | only https:// URLs are supported")

This would make the S310 suppression more clearly justified.

Issue: There's no test for the fallback case on line 113 where a GitHub URL has more than 2 path segments (e.g., https://github.com/org/repo/something-unexpected) and falls through to return the URL as-is. This is the "Unrecognised GitHub URL pattern" branch. Codecov also reports some missing coverage in this file.

Suggestion: Add a test case like:

def test_unrecognized_github_path(self):
    url = "https://github.com/org/repo/wiki/Some-Page"
    assert resolve_to_raw_url(url) == url

I'm don't think we should be doing the raw url translation. I'd say this is an application concern, that they should pass correct URLs to the Skill.from_url

This PR should be targeted at loading skills from URL, should not be opinionated on what that url is. At the end of the day, it's up to the devs to pass in the correct url

Skill.from_url should essentially be: get url as input, make GET request to get content from the url, pass it down to Skill.from_content. We should not have any complex resolution

Done — removed all the GitHub URL resolution logic in the latest push. Skill.from_url() is now just: validate https://, GET the URL, pass to from_content(). ~65 lines total in _url_loader.py, no opinions on URL format.

do we explicitly need url loader import in method? if not, can we move to top?

we should not have imports in methods, unless necessary. let's move this to the top

nit: do we need another file? can we just move this under Skill.from_url (or like as helper methods there)?